Show more request context in the UI (fix #69) #105
Conversation
|
As discussed in the past, totally agree on adding the caller identity. I appreciate this is a reference implementation, but IMO there's value in bringing our recommendations/best practices on wording, hence sharing a bit more feedback:
How about something like:
And:
Figmas for other platforms from passkeycentral.org. @msirringhaus, @iinuwa, WDYT? @timcappalli I don't know of any published guidelines for platforms, but would be curious to hear your thoughts if you have any, or if you know someone who might. |
|
I added more context now. We can debate if we need PID, binary path and application name, but I have put everything in there now, to see how it looks. I also tried to make the UI a bit cleaner by adding some spacing and separators. Still far from a polished UI, but better than nothing. |
msirringhaus
force-pushed
the
ui_more_request_context
branch
from
9945cd6 to
dec5b0f
Compare
…lly up to a given limit instead of giving them a big minimal height
|
@AlfioEmanueleFresta I think your proposed UI/text makes sense and nicely aligns with other experiences in the ecosystem, while also addressing unique considerations for Linux (showing process ID in addition to name). |
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
I had a draft about this a couple weeks ago but I've lost it. I'll try to recover it.
I think this is fine as is (besides the changes mentioned below). Because of namespaces, all of the information shown can be spoofed by an attacker, but it's the best we can do for now. We can revisit in the future.
After migrating to use PIDFDs instead, we can merge this. Any more UI polish can happen later.
Thank you!
credentialsd/src/dbus/gateway.rs
Outdated
| let sender_unique_name = header.sender()?; | ||
|
|
||
| tracing::debug!("Received request from sender: {}", sender_unique_name); | ||
|
|
||
| // 2. Use the connection to query the D-Bus daemon for more info | ||
| let proxy = match zbus::Proxy::new( | ||
| connection, | ||
| "org.freedesktop.DBus", | ||
| "/org/freedesktop/DBus", | ||
| "org.freedesktop.DBus", | ||
| ) | ||
| .await | ||
| { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| tracing::error!("Failed to establish DBus proxy to query peer info: {e:?}"); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| // 3. Get the Process ID (PID) of the peer | ||
| let pid_result = match proxy | ||
| .call_method("GetConnectionUnixProcessID", &(sender_unique_name)) | ||
| .await | ||
| { | ||
| Ok(pid) => pid, | ||
| Err(e) => { | ||
| tracing::error!("Failed to get peer PID via DBus: {e:?}"); | ||
| return None; | ||
| } | ||
| }; | ||
| let pid: u32 = match pid_result.body().deserialize() { | ||
| Ok(pid) => pid, | ||
| Err(e) => { | ||
| tracing::error!("Retrieved peer PID is not an integer: {e:?}"); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| // 4. Get binary path via PID from /proc file-system | ||
| // TODO: To be REALLY sure, we may want to look at /proc/PID/exe instead. It is a symlink to | ||
| // the actual binary, giving a full path instead of only the command name. | ||
| // This should in theory be "more secure", but also may disconcert novice users with no | ||
| // technical background. | ||
| let command_name = match std::fs::read_to_string(format!("/proc/{pid}/comm")) { | ||
| Ok(c) => c.trim().to_string(), | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Failed to read /proc/{pid}/comm, so we don't know the command name of peer: {e:?}" | ||
| ); | ||
| return None; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Can we use connection.peer_credentials.process_fd() here instead? Using a pidfd is safer against certain kinds of attacks.
Something like this (with proper error handling) should work to parse the PID from that pidfd:
let pidfd = connection.peer_credentials.process_fd()
.expect("pidfd to be retrieved from DBus connection")
.into_raw_fd();
let fdinfo_str = std::fs::read_to_string(format!("/proc/self/fdinfo/{pidfd}"))
.map_err(|err| format!("Failed to read fdinfo from procfs: {err}"))?;
let pid_from_pidfd: pid_t = fdinfo_str
.split("\n")
.map(|line| line.split_once(":\t").expect("fdinfo text to be returned"))
.find(|(k, _)| *k == "Pid")
.expect("Pid field to exist in fdinfo")
.1
.parse()
.expect("valid PID from fdinfo");I think there's also a recently added PIDFD_INFO_PID ioctl to make this shorter, but I don't know when that was added to the kernel or if it was backported. This should be compatible back to 5.3.
4 participants
Note: This is based on the translations-PR to utilize translatable strings here, too. So for now, only look at the last commit.
Points of contention:
/proc/PID/commis used for the application name, instead of the full absolute path to the binary. I think the full path may confuse more than help novice users, but I'm open to change that.If we stay with the current proposal, I would need to add more translations-metadata to make it clear to potential translators how the new strings are used.